-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cranelift: Make heap_addr
return calculated base + index + offset
#5231
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
cranelift
Issues related to the Cranelift code generator
cranelift:meta
Everything related to the meta-language.
labels
Nov 9, 2022
jameysharp
reviewed
Nov 9, 2022
fitzgen
force-pushed
the
update-heap-addr
branch
from
November 9, 2022 17:52
4542a8f
to
adb2d6c
Compare
Rather than return just the `base + index`. (Note: I've chosen to use the nomenclature "index" for the dynamic operand and "offset" for the static immediate.) This move the addition of the `offset` into `heap_addr`, instead of leaving it for the subsequent memory operation, so that we can Spectre-guard the full address, and not allow speculative execution to read the first 4GiB of memory. Before this commit, we were effectively doing load(spectre_guard(base + index) + offset) Now we are effectively doing load(spectre_guard(base + index + offset)) Finally, this also corrects `heap_addr`'s documented semantics to say that it returns an address that will trap on access if `index + offset + access_size` is out of bounds for the given heap, rather than saying that the `heap_addr` itself will trap. This matches the implemented behavior for static memories, and after bytecodealliance#5190 lands (which is blocked on this commit) will also match the implemented behavior for dynamic memories.
fitzgen
force-pushed
the
update-heap-addr
branch
from
November 9, 2022 17:56
adb2d6c
to
9996cb1
Compare
cfallin
approved these changes
Nov 9, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally really good to me -- thanks for doing this update!
I added some nitpicks on the documentation, some on pre-existing stuff, as I figure it's worth trying to get this really polished and clear; and a few little cleanups elsewhere; but nothing major.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
cranelift:meta
Everything related to the meta-language.
cranelift
Issues related to the Cranelift code generator
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than return just the
base + index
.(Note: I've chosen to use the nomenclature "index" for the dynamic operand and "offset" for the static immediate.)
This move the addition of the
offset
intoheap_addr
, instead of leaving it for the subsequent memory operation, so that we can Spectre-guard the full address, and not allow speculative execution to read the first 4GiB of memory.Before this commit, we were effectively doing
Now we are effectively doing
Finally, this also corrects
heap_addr
's documented semantics to say that it returns an address that will trap on access ifindex + offset + access_size
is out of bounds for the given heap, rather than saying that theheap_addr
itself will trap. This matches the implemented behavior for static memories, and after #5190 lands (which is blocked on this commit) will also match the implemented behavior for dynamic memories.